Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Remove langchain from index operations #827

Merged
merged 10 commits into from
May 9, 2024
Merged

refactor: Remove langchain from index operations #827

merged 10 commits into from
May 9, 2024

Conversation

adamdougal
Copy link
Collaborator

@adamdougal adamdougal commented May 7, 2024 •
edited
Loading

  • This is preparation for adding an additional image vector field for
    advanced image processing
  • I've tried to keep changes to a minimum
  • Still using langchain in the question and answer tool for now
  • Increased unit test coverage

Required by #748

Testing

  • Fresh deployment
  • On top of an existing deployment
  • Integrated vectorisation

@adamdougal adamdougal mentioned this pull request May 7, 2024
Closed
7 tasks
@github-actions GitHub Actions
Copy link

github-actions bot commented May 7, 2024 •
edited
Loading

Coverage

Coverage Report •
FileStmtsMissCoverMissing
code/backend/batch/utilities/common
   SourceDocument.py60591%44, 47, 51, 55, 128
code/backend/batch/utilities/helpers
   AzureSearchHelper.py470100% 
   LLMHelper.py421173%40–41, 50, 61–62, 73, 86–87, 94, 113, 121
code/backend/batch/utilities/helpers/embedders
   PushEmbedder.py460100% 
code/backend/batch/utilities/search
   AzureSearchHandler.py56198%24
   IntegratedVectorizationSearchHandler.py510100% 
   Search.py13192%15
   SearchHandlerBase.py27774%13, 17, 21, 25, 29, 33, 37
code/backend/batch/utilities/tools
   QuestionAnswerTool.py610100% 
TOTAL228768969% 

Tests Skipped Failures Errors Time
172 0 💤 0 ❌ 0 🔥 10.711s ⏱️

@superhindupur
Copy link
Contributor

Sidebar: could you mark the PR as draft if you don't want it merged? Not everyone might notice the DO NOT MERGE 😆

@adamdougal adamdougal force-pushed the adam/748/remove-langchain-from-index-ops branch from 56c29ee to cbb3d08 Compare May 7, 2024 17:40
cecheta
cecheta reviewed May 8, 2024
View reviewed changes
Copy link
Collaborator

@cecheta cecheta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work

code/backend/batch/utilities/search/SearchHandlerBase.py Show resolved Hide resolved
code/backend/batch/utilities/tools/QuestionAnswerTool.py Outdated Show resolved Hide resolved
code/tests/functional/backend_api/conftest.py Outdated Show resolved Hide resolved
code/tests/utilities/helpers/processors/test_IntegratedVectorizationEmbedder.py Outdated Show resolved Hide resolved
@adamdougal adamdougal changed the title Remove langchain from index operations refactor: Remove langchain from index operations May 8, 2024
@adamdougal adamdougal requested review from cecheta and gaurarpit May 8, 2024 16:38
@adamdougal adamdougal marked this pull request as draft May 9, 2024 08:20
adamdougal added 7 commits May 9, 2024 09:39
@adamdougal
Remove langchain from index operations
40c9885 
- This is preparation for adding an additional image vector field for
  advanced image processing
- I've tried to keep changes to a minimum
- Still using langchain in the question and answer tool for now
- Increased unit test coverage

Required by #748
@adamdougal
Fix after merge
ed97f2f
@adamdougal
Fix import
d7d8546
@adamdougal
Stop catching and rethrowing exception
0522fb1
@adamdougal
Typo
a3d8eab
@adamdougal
Explain oneshot request
365e5a2
@adamdougal
Yield llmhelper
60a8365
@adamdougal adamdougal force-pushed the adam/748/remove-langchain-from-index-ops branch from 4ce83b4 to 60a8365 Compare May 9, 2024 08:39
@adamdougal adamdougal marked this pull request as ready for review May 9, 2024 10:17
gaurarpit
gaurarpit approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@gaurarpit gaurarpit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Ship-it.

           (`-,-,
           ('(_,( )
            _   `_'
         __|_|__|_|_
       _|___________|__
      |o o o o o o o o/   
     ~'`~'`~'`~'`~'`~'`~

@adamdougal adamdougal added this pull request to the merge queue May 9, 2024
@adamdougal adamdougal removed this pull request from the merge queue due to a manual request May 9, 2024
@adamdougal adamdougal added this pull request to the merge queue May 9, 2024
Merged via the queue into main with commit 6dd9f61 May 9, 2024
9 checks passed
@adamdougal adamdougal deleted the adam/748/remove-langchain-from-index-ops branch May 9, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cecheta cecheta cecheta approved these changes

@gaurarpit gaurarpit gaurarpit approved these changes

@ross-p-smith ross-p-smith Awaiting requested review from ross-p-smith

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adamdougal @superhindupur @gaurarpit @cecheta